fix(sqlserver): preserve IDENTITY columns when copying table (#1691)#1918
fix(sqlserver): preserve IDENTITY columns when copying table (#1691)#1918HandSonic wants to merge 1 commit into
Conversation
openai0229
left a comment
There was a problem hiding this comment.
The current implementation is not safe for the SQL Server table shapes claimed by this PR.
SET IDENTITY_INSERT ... ONis executed unconditionally and fails for tables without an identity column.INSERT INTO target SELECT * FROM sourceis invalid when computed columns or rowversion/timestamp columns are present; the copy needs explicit compatible column lists.- Global
ddl.replace("[source]", "[target]")can rewrite references outside the object name and does not safely regenerate constraint/index names.
Please redesign this around parsed metadata and explicit column/object lists, then add SQL Server integration tests for tables with no identity, identity, computed columns, rowversion, defaults, constraints, and indexes. The PR should not claim those objects are preserved until the tests prove it.
8bbb63c to
06fb88f
Compare
|
Redesigned the copyTable implementation per review feedback: 1. Conditional IDENTITY_INSERT — no longer executed unconditionally. Now queries 2. Explicit column list — instead of
Uses 3. Safe table name replacement — instead of global 4. Rebased on current main. |
…nd#1691) Use Chat2DBContext.getDbMetaData().tableDDL() to generate proper CREATE TABLE DDL that preserves IDENTITY, computed columns, constraints, indexes, etc. For data copying, wrap INSERT INTO...SELECT with SET IDENTITY_INSERT ON/OFF to allow explicit values in identity columns. The previous SELECT * INTO approach silently dropped IDENTITY properties.
06fb88f to
3e9f21a
Compare
Use tableDDL() to generate proper CREATE TABLE DDL that preserves IDENTITY, computed columns, constraints, and indexes.
For data copying, wrap INSERT INTO...SELECT with SET IDENTITY_INSERT ON/OFF to allow explicit values in identity columns.
The previous SELECT * INTO approach silently dropped IDENTITY properties.
Fixes #1691